-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update MP REST Client to 4.0 #43959
base: main
Are you sure you want to change the base?
Update MP REST Client to 4.0 #43959
Conversation
🎊 PR Preview de27583 has been successfully built and deployed to https://quarkus-pr-main-43959-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It looks mostly good to me except for some minor suggestions.
I would prefer for @geoand to have a look though.
...src/main/java/io/quarkus/rest/client/reactive/runtime/DefaultClientHeadersRequestFilter.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Outdated
Show resolved
Hide resolved
...asy-client/runtime/src/main/java/io/quarkus/restclient/runtime/QuarkusRestClientBuilder.java
Show resolved
Hide resolved
2e2996e
to
95b1469
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
95b1469
to
9028324
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Guillaume Smet <[email protected]>
9028324
to
6c4a5b6
Compare
<dependency> | ||
<groupId>org.jboss.resteasy</groupId> | ||
<artifactId>resteasy-multipart-provider</artifactId> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I mind, but I would like to know why this is now necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if it's necessary, we might have to depend on the quarkus-resteasy-multipart
extension instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the dependency from RESTEasy that provides the EntityPart.Builder
implementation. Note that for the REST Client Classic, I've tried to keep it as the original implementation: resteasy/resteasy-microprofile@bb8b433
return Stream.of(type.getMethods()) | ||
.filter(method -> !IGNORED_METHODS.contains(method)) | ||
.toArray(Method[]::new); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely no fan of this, but we can let it go in :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a copy from the original RESTEasy Builder code:
resteasy/resteasy-microprofile@cb58ceb
Initially, we used the one provided by RESTEasy, but we made a copy to remove references to their CDI integration and also to remove the CDI dependency. They diverged a bit, so I've tried to reconcile them again for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
Status for workflow
|
Status for workflow
|
No description provided.